Fix `YAMLDecoder`'s handling when decoding empty strings into optional types #351

liamnichols · 2022-04-06T11:41:05Z

Background

I was checking the feasibility of using YAMLDecoder to decode a simple JSON file instead of having to switch between JSONDecoder, but I noticed a discrepancy in a really simple example:

let json = """
    {
      "access": ""
    }
    """

struct Container: Decodable {
    let access: String?
}

let decoded = try YAMLDecoder().decode(Container.self, from: json)
XCTAssertEqual(decoded.access, "") // failed: ("nil") is not equal to ("Optional("")")

This was odd, because the problem does not surface when using Yams.load(yaml:):

let json = """
    {
      "access": ""
    }
    """

let decoded = try XCTUnwrap(try Yams.load(yaml: json) as? NSDictionary)
XCTAssertEqual(decoded, ["access": ""]) // success

It was then where I discovered #301.

Description

Disclaimer: This is my first time ever looking at this codebase, so I likely will have missed something 🙏

Following the hints from @JensAyton, I could see in NSNull.construct(from:) that we weren't factoring in the style of the input scalar, which meant that technically this method would treat 'null' or '' as null. This however never seemed to surface as an issue when using Yams.load(yaml:) since the calling methods would perform additional checks on things like the node tag. _Decoder.decodeNil() on the other hand does not.

While I could have implemented a fix in _Decoder.decodeNil(), I felt that it might be more appropriate to put it in NSNull.construct(from:), which I did here.

In addition to the fix, I added extra test coverage to reproduce the issue and I also had to make one small adjustment to NodeTests due to this change.. I'll add a comment inline.

Finally, I also want to reiterate another comment from @JensAyton:

However, I’m leery of making a PR with this change since it may impact existing code with unfortunate dependencies on the current behaviour.

I feel the same way. I'd very much appreciate thorough eyes on this. I've added what I think is appropriate test coverage, but if you think I need to do more, or if there is a better approach to fix this, please let me know 🙏

liamnichols

Some inline commentary 🙏

liamnichols · 2022-04-06T11:42:28Z

Tests/YamsTests/ConstructorTests.swift

-            # This sequence has five
-            # entries, two have values.
+            # This sequence has seven
+            # entries, four have values.
            sparse:
              - ~
              - 2nd entry
              -
              - 4th entry
              - Null
+              - 'null'
+              - ''


Before the change to NSNull.construct(yaml:), these tests passed anyway. This is because of other stuff that is done when calling Yams.load_all(yaml:). I just made the changes here anyway to highlight the expectation incase things change in the future.

liamnichols · 2022-04-06T11:45:04Z

Tests/YamsTests/TopLevelDecoderTests.swift

+        XCTAssertEqual(container.A, "")
+        XCTAssertEqual(container.B, nil)
+        XCTAssertEqual(container.C, nil)
+        XCTAssertEqual(container.D, nil)
+        XCTAssertEqual(container.E, "")
+        XCTAssertEqual(container.json.F, "")
+        XCTAssertEqual(container.json.G, "null")
+        XCTAssertEqual(container.array, ["one", "", nil, "null", "~"])


without the patch, these are the failures:

XCTAssertEqual(container.A, "") // failed: ("nil") is not equal to ("Optional("")") XCTAssertEqual(container.B, nil) XCTAssertEqual(container.C, nil) XCTAssertEqual(container.D, nil) XCTAssertEqual(container.E, "") // failed: ("nil") is not equal to ("Optional("")") XCTAssertEqual(container.json.F, "") // failed: ("nil") is not equal to ("Optional("")") XCTAssertEqual(container.json.G, "null") // failed: ("nil") is not equal to ("Optional("null")") XCTAssertEqual(container.array, ["one", "", nil, "null", "~"]) // failed: ("[Optional("one"), nil, nil, nil, nil]") is not equal to ("[Optional("one"), Optional(""), nil, Optional("null"), Optional("~")]")

liamnichols · 2022-04-06T11:47:22Z

Tests/YamsTests/NodeTests.swift

-        let scalarNull: Node = "null"
+        let scalarNull = Node("null", .implicit, .plain)
        XCTAssertEqual(scalarNull.null, NSNull())


I had to make this change here since the construct method will no longer construct NSNull if the style isn't .plain.

Have I missed anything here, is that ok? I'm not familiar how this is used elsewhere but I think its ok? Do we have to worry about styles like .any? Or is that more only for when using?

This is the only thing that makes me wonder if the patch should have been inside _Decoder.decodeNil() instead as I'm not sure if it was intentional or not?

Let me know if there are better tests I can add to verify this.

liamnichols · 2022-04-11T08:32:12Z

I checked out the CI failures and corrected some SwiftLint violations that it was erroring on, but I'm not too sure why the rest of the CI jobs failed. If you have any suggestions on what I can do to debug/replicate this locally that will help. Thanks

…#301

…ng of null specifiers that are wrapped in quotes

….plain since non-plain style scalars don't represent null (fixes bug)

… making assertion

liamnichols · 2022-06-09T20:49:40Z

@jpsim I've just rebased this PR with the latest changes from main and I was just wondering if you could please trigger the CI checks again? 🙏

mattpolzin · 2023-10-12T18:45:37Z

@jpsim the issue this PR attempts to fix is a bit tricky to work around downstream of the parsing (sometimes impossible without accepting both things you want to parse successfully and also things you don't want to parse successfully).

If you get the chance to revisit the issue, I'd love to know whether this PR (assuming conflicts were fixed) has some chance of getting merged, or whether you'd prefer a different solution, or whether you don't anticipate accepting a fix for the associated bug now or in the future.

jpsim · 2023-10-15T18:46:00Z

Hi everyone, really sorry for the long delay here, but I've merged these changes in 0c4ff78 and will be cutting a new major release in the next few days, considering that this may be a breaking change for some users.

Thanks for everyone's contributions and patience here. ❤️

mattpolzin · 2023-10-15T18:48:53Z

Thanks @jpsim! Really appreciate you looping back on this, not to mention all the other effort you've put into Yams.

liamnichols · 2023-10-16T08:50:20Z

Thanks @jpsim, much appreciated 🙏

liamnichols commented Apr 6, 2022

View reviewed changes

This was referenced Apr 6, 2022

Unstable order when reading spec from .json file CreateAPI/CreateAPI#12

Closed

Decode JSON input specs using YAMLDecoder CreateAPI/CreateAPI#34

Merged

liamnichols added 5 commits June 9, 2022 22:47

Add TopLevelDecoderTests.testDecodeOptionalTypes() to reproduce jpsim…

0de508f

…#301

Update ConstructorTests.testNull() to include assertions about handli…

89679a2

…ng of null specifiers that are wrapped in quotes

Update NSNull.construct(from:) to only return NSNull if the style is …

3aaed24

….plain since non-plain style scalars don't represent null (fixes bug)

Update assertion in NodeTests to explicitly require .plain style when…

94ad299

… making assertion

Correct SwiftLint violations

108a304

liamnichols force-pushed the ln/decoding-empty-string-optionals branch from f8f249b to 108a304 Compare June 9, 2022 20:47

liamnichols mentioned this pull request Jun 9, 2022

Fix YAMLDecoder's handling when decoding empty strings into optional types CreateAPI/Yams#1

Merged

czechboy0 mentioned this pull request Jul 14, 2023

Nullable enums with an empty string fail to get generated apple/swift-openapi-generator#118

Closed

czechboy0 mentioned this pull request Oct 11, 2023

Swift OpenAPI Generator Fails to Decode anyOf Structure for nullable Field apple/swift-openapi-generator#286

Open

jpsim closed this Oct 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `YAMLDecoder`'s handling when decoding empty strings into optional types #351

Fix `YAMLDecoder`'s handling when decoding empty strings into optional types #351

liamnichols commented Apr 6, 2022 •

edited

Loading

liamnichols left a comment

liamnichols Apr 6, 2022

liamnichols Apr 6, 2022

liamnichols Apr 6, 2022

liamnichols commented Apr 11, 2022

liamnichols commented Jun 9, 2022

mattpolzin commented Oct 12, 2023

jpsim commented Oct 15, 2023

mattpolzin commented Oct 15, 2023

liamnichols commented Oct 16, 2023

Fix YAMLDecoder's handling when decoding empty strings into optional types #351

Fix YAMLDecoder's handling when decoding empty strings into optional types #351

Conversation

liamnichols commented Apr 6, 2022 • edited Loading

Background

Description

liamnichols left a comment

Choose a reason for hiding this comment

liamnichols Apr 6, 2022

Choose a reason for hiding this comment

liamnichols Apr 6, 2022

Choose a reason for hiding this comment

liamnichols Apr 6, 2022

Choose a reason for hiding this comment

liamnichols commented Apr 11, 2022

liamnichols commented Jun 9, 2022

mattpolzin commented Oct 12, 2023

jpsim commented Oct 15, 2023

mattpolzin commented Oct 15, 2023

liamnichols commented Oct 16, 2023

Fix `YAMLDecoder`'s handling when decoding empty strings into optional types #351

Fix `YAMLDecoder`'s handling when decoding empty strings into optional types #351

liamnichols commented Apr 6, 2022 •

edited

Loading